Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add branch install notes to readme (plus clean up) #41

Merged
merged 34 commits into from
Sep 4, 2024
Merged

Add branch install notes to readme (plus clean up) #41

merged 34 commits into from
Sep 4, 2024

Conversation

cjrace
Copy link
Contributor

@cjrace cjrace commented Jul 19, 2024 •
edited
Loading

Added a note to the readme about how to install from a branch as I keep forgetting, then spiralled into also:

  • Breaking off a contributing file for the package (away from the readme, so notes for maintainers go in there and then the readme is user facing)
  • Add a pull request template from dfeR
  • Update the top of the readme to be more similar to dfeR
  • Change lintr settings to only check for lines over 100 characters, which is what CRAN uses for PDF documentation (and added lintr config file to buildignore to hush the package checks)
  • Fixed some lintr warnings
  • Upgraded roxygen2 to the latest
  • Updated to meet community standards as in Complete community standards checklist #28
  • Generated favicons using pkgdown::build_favicons() fixing Make favicon work #36

I've also added a GitHub action for checking the test_dashboard while it's still separate to the main package checks. This means even if we forget to run, it will always be there on PRs. I've double checked this will show as a fail too (can see in the commit history).

@cjrace cjrace changed the title Add branch install readme Add branch install notes to readme Jul 19, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 19, 2024
View reviewed changes
R/cookies.R Fixed Show fixed Hide fixed
R/cookies.R Fixed Show fixed Hide fixed
@cjrace cjrace mentioned this pull request Jul 19, 2024
Closed
@cjrace cjrace linked an issue Jul 19, 2024 that may be closed by this pull request
lintr action is incorrectly flagging missing bindings #31
Closed
@cjrace cjrace mentioned this pull request Jul 19, 2024
Closed
@cjrace cjrace linked an issue Jul 19, 2024 that may be closed by this pull request
Make test_dashboard tests run in the main checks #23
Closed
This was linked to issues Jul 19, 2024
Make favicon work #36
Closed
Complete community standards checklist #28
Closed
@cjrace cjrace changed the title Add branch install notes to readme Add branch install notes to readme (plus clean up) Jul 19, 2024
@cjrace cjrace marked this pull request as ready for review July 19, 2024 17:38
@cjrace cjrace requested a review from rmbielby July 19, 2024 17:39
@cjrace
Copy link
Contributor Author

cjrace commented Jul 23, 2024

Using this as a mop up PR, some thoughts on potential other things we might want to sort before we comment to bumping the version and more people adopt this, interested in your thoughts @rmbielby:

  1. I feel a little uneasy with us pointing at a file on then main branch for analytics.html and cookie-consent.js, I'd like to either look at making it package data, or, putting them into their own function scripts that can be called by the init ones (or have inline) so that the code is available on branch always
  2. We don't have any tests on either of the init scripts, feels like we should be checking those are at least writing lines (the above will help with this)
  3. We mix up our references to cookie... v cookies..., changing now could break existing functions, but while adoption is low it could be worth a sweep around forcing all references to be consistently singular or plural to reduce the likelihood of typos from ourselves and users costing time later? If so, any preference on which way?
  4. For the panel functions we're currently tying ourselves into the shiny::navlistPanel() layout, given we're likely to change that, would it be work breaking the support_panel() function and the cookies panel to just return a div of the content and then in examples we change to wrap the navigation around it?

Happy to pick these up / share out once we agree an approach

@rmbielby
Copy link
Contributor

Any idea on what CRAN policies are in place around the script-copying type stuff? Feel like this might also be seen as a potential vulnerability if we do it the wrong way. Will try and do some scouting to see if it's anything that would be picked up by their package requirements.

@cjrace
Copy link
Contributor Author

cjrace commented Jul 23, 2024

Any idea on what CRAN policies are in place around the script-copying type stuff? Feel like this might also be seen as a potential vulnerability if we do it the wrong way. Will try and do some scouting to see if it's anything that would be picked up by their package requirements.

I'm not sure, which is exactly why I'm feeling un-EES-y about it!

This was referenced Jul 31, 2024
Merged
Merged
@cjrace cjrace mentioned this pull request Jul 31, 2024
Merged
@cjrace
Copy link
Contributor Author

cjrace commented Aug 2, 2024

Using this as a mop up PR, some thoughts on potential other things we might want to sort before we comment to bumping the version and more people adopt this, interested in your thoughts @rmbielby:

  1. I feel a little uneasy with us pointing at a file on then main branch for analytics.html and cookie-consent.js, I'd like to either look at making it package data, or, putting them into their own function scripts that can be called by the init ones (or have inline) so that the code is available on branch always
  2. We don't have any tests on either of the init scripts, feels like we should be checking those are at least writing lines (the above will help with this)
  3. We mix up our references to cookie... v cookies..., changing now could break existing functions, but while adoption is low it could be worth a sweep around forcing all references to be consistently singular or plural to reduce the likelihood of typos from ourselves and users costing time later? If so, any preference on which way?
  4. For the panel functions we're currently tying ourselves into the shiny::navlistPanel() layout, given we're likely to change that, would it be work breaking the support_panel() function and the cookies panel to just return a div of the content and then in examples we change to wrap the navigation around it?

Happy to pick these up / share out once we agree an approach

Have raised #42, #43 and #44 covering these changes, they all point at this branch

cjrace added 3 commits August 7, 2024 15:21
@cjrace
Part 1 - update cookie to cookies (#42)
13d2188 
* update cookie to cookies

* run r cmd check on all pull requests
@cjrace
Part 2 - Make init code inline (#44)
5593479 
* Update init_analytics to have a create_file option and add more unit tests around the content

* Move the init_analytics HTML / JS to be written inline

* Prevent init_analytics from running in examples and creating scripts in the check directory

* update init_cookies to have the JS code inline and add some tests
@cjrace
Part 3 - remove tab dependencies (#43)
8acf6aa 
* update cookie to cookies

* remove tabPanel from within our functions

* remove images from snapshots as not needed

* Switch support panel tests to unit tests (as the UI ones didn't track anything beyond if the tab was selected)

* Add tests for cookies_panel_ui

* Add note to contributing guidelines about lintr and loading package first

* run r cmd check on all PRs

* force some extra waits
@cjrace cjrace requested a review from rmbielby August 29, 2024 14:42
@cjrace cjrace assigned rmbielby and unassigned cjrace Aug 29, 2024
rmbielby
rmbielby reviewed Sep 2, 2024
View reviewed changes
Copy link
Contributor

@rmbielby rmbielby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment

.github/CONTRIBUTING.md Outdated Show resolved Hide resolved
rmbielby
rmbielby requested changes Sep 2, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@cjrace cjrace requested review from rmbielby and removed request for chfoster and jen-machin September 3, 2024 18:10
@cjrace cjrace merged commit 48f848d into main Sep 4, 2024
10 checks passed
@cjrace cjrace deleted the add-branch-install-readme branch September 4, 2024 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rmbielby rmbielby rmbielby approved these changes

Assignees

@rmbielby rmbielby

Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@cjrace @rmbielby